CORE-2731: Generate a global CSS token file from the theme - #3133
Open
OpenStaxClaude wants to merge 15 commits into
Open
CORE-2731: Generate a global CSS token file from the theme#3133OpenStaxClaude wants to merge 15 commits into
OpenStaxClaude wants to merge 15 commits into
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/app/content/components/BookBanner.css:104
z-indexexpects an , andcalc(var(--z-index-navbar) - 1)is not valid for integer-typed properties in CSS; browsers may drop the declaration, changing stacking order. Consider computing these numbers in JS and binding them via component-scoped custom properties (e.g.--book-banner-z-index-big/mini), or hardcoding the integer values if you want to avoid extra bindings.
src/test/cssColors.ts:413stylesheetColorsrebuilds a new array on every declaration via spread inreduce, which is O(n²) allocations for large stylesheets and makes the audit unnecessarily slow/heavy in Jest and baseline generation. Build the result incrementally withpushinstead.
RoyEJohnson
marked this pull request as ready for review
September 1, 2026 21:03
The comparison the two locked checks are built on had no tests, so a bug in it would not fail anything -- it would quietly stop the checks catching what they exist to catch, which is the one failure mode a lockfile-style guard cannot afford. Five cases, of which the two multiset ones are the point: a set-based implementation of this helper passes the other three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Port of the fix ui-components#143 made for --ox-color-link, which applies here at a larger scale. themeColorIndex kept one token per color value, so whichever came last in the projection was reported as *the* replacement. Ten of the theme's colors are carried by more than one token -- #fff by eleven, #424242 by six, #000 by five. It was already wrong in the committed baseline: every #000 was attributed to --color-text-black, including .nudge-click-blocker's background-color, a full-screen overlay, and .button-primary:focus's box-shadow. Neither is text and both mean --color-black, so a developer following the message would have written the wrong token and the check would have passed -- worse than saying nothing. It now names all of them and the author picks, ordered least-qualified-name first so #fff leads with --color-white rather than burying it behind nine *-foreground tokens. Also takes the suggestion out of the baseline entry. Appending it made the advice part of the identity, so renaming a token, or giving an existing value a new token, would rewrite every affected line -- churn for a reason unconnected to the CSS, which is where this PR argues a new color hides. Entries are identity only now and the tokens are attached to `added` when a check fails. Counts unchanged at 206/37; the baseline diff is nothing but the dropped suffix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
This was referenced Sep 8, 2026
Merged
TomWoodward
temporarily deployed
to
rex-web-core-2731-globa-fydzwf
September 10, 2026 18:16
Inactive
src/test/cssColors.ts is now pure CSS parsing with no imports at all: given stylesheet text, which color literals are in it and which declaration each was written in. Everything that knows about REX -- the theme token index, the off-palette allowlist, the stylesheet walk and the violation report -- moves to src/test/themeColors.ts, which is what theme.spec.ts and the baseline generator use. The boundary already existed; only the file did not. fs and path were used by the REX half alone, and the engine's half of the spec never needed the theme. Splitting it lets the parser be reviewed on its own, which is what Roy asked for on #3133. No behaviour change: theme.baseline.json regenerates byte-identical.
Pure CSS parsing that answers one question: given stylesheet text, which color
literals are in it and which declaration each one was written in. It knows
nothing about REX, the theme or the token file -- that layer, and everything
that enforces it, is the stacked PR.
- stripNoise blanks comments, string contents and url() payloads without
changing length, so a second differently-blanked copy can be indexed in step
with the first: structure is read where strings are gone, context is sliced
where they survive.
- A brace/semicolon scan pulls {context, property, value} at any nesting depth,
so @media is covered while selectors and @Keyframes percentages are not read
as values.
- Each value is walked for colors in any syntax. Functions that merely contain
colors (var(), color-mix(), the gradients) are descended into; the color
functions are terminal. A bare identifier is only read as a color where one
can legally go, so animation-name: red is not a finding while hex and the
color functions stay in scope everywhere. Anything that cannot be resolved to
channels comes back as rgba: null rather than passing silently.
96 cases pin the contract in both directions -- what it must flag and what it
must leave alone.
…-css-theme-tokens
OpenStaxClaude
changed the base branch from
main
to
CORE-2731-css-color-audit-engine
September 11, 2026 15:21
This comment was marked as resolved.
This comment was marked as resolved.
RoyEJohnson
added this pull request to stack #3143
September 11, 2026 15:29
OpenStaxClaude
added a commit
that referenced
this pull request
Sep 11, 2026
Each was reported by Copilot on #3142, and each already had a fix on ui-components#149, so these are ports of those rather than new inventions -- the two copies converge ahead of CORE-2737 swapping this file for the published engine. - device-cmyk() is a terminal color function. Absent from the list it was descended into, its numeric arguments matched nothing, and a hardcoded color passed the audit unreported. - `url(` now needs an ident boundary. `myurl(#fff)` was read as a url token and its payload blanked, losing the color. - The url scan is quote- and escape-aware. `url("asset).svg")` closed at the paren in the filename, leaving the trailing quote to open an unterminated string that blanked the rest of the stylesheet. - Selector whitespace collapses only outside strings, so `[data-value="a b"]` and `[data-value="a b"]` stay distinct -- which is the reason the context copy keeps its strings at all. - Channels and alpha use the CSS <number> grammar. `[\d.]+` also matched `.` and `1..2`, which parseFloat turned into NaN and a truncated 1 and which were handed back as resolved -- the failure the hex grammar check already prevents. It also rejected the legal `+255` and `2.55e2`. - The rgb()/rgba() grammar crosses newlines. Upstream uses the `s` flag; REX targets es2017, where that is a compile error, so this uses [\s\S]. 18 spec cases, 13 of which fail against the previous parser. None of these constructs appear in REX's stylesheets, so theme.baseline.json on #3133 is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Sep 11, 2026
* CORE-2731: add the CSS color audit engine
Pure CSS parsing that answers one question: given stylesheet text, which color
literals are in it and which declaration each one was written in. It knows
nothing about REX, the theme or the token file -- that layer, and everything
that enforces it, is the stacked PR.
- stripNoise blanks comments, string contents and url() payloads without
changing length, so a second differently-blanked copy can be indexed in step
with the first: structure is read where strings are gone, context is sliced
where they survive.
- A brace/semicolon scan pulls {context, property, value} at any nesting depth,
so @media is covered while selectors and @Keyframes percentages are not read
as values.
- Each value is walked for colors in any syntax. Functions that merely contain
colors (var(), color-mix(), the gradients) are descended into; the color
functions are terminal. A bare identifier is only read as a color where one
can legally go, so animation-name: red is not a finding while hex and the
color functions stay in scope everywhere. Anything that cannot be resolved to
channels comes back as rgba: null rather than passing silently.
96 cases pin the contract in both directions -- what it must flag and what it
must leave alone.
* CORE-2731: fix six parser edge cases found in review
Each was reported by Copilot on #3142, and each already had a fix on
ui-components#149, so these are ports of those rather than new
inventions -- the two copies converge ahead of CORE-2737 swapping this
file for the published engine.
- device-cmyk() is a terminal color function. Absent from the list it
was descended into, its numeric arguments matched nothing, and a
hardcoded color passed the audit unreported.
- `url(` now needs an ident boundary. `myurl(#fff)` was read as a url
token and its payload blanked, losing the color.
- The url scan is quote- and escape-aware. `url("asset).svg")` closed
at the paren in the filename, leaving the trailing quote to open an
unterminated string that blanked the rest of the stylesheet.
- Selector whitespace collapses only outside strings, so
`[data-value="a b"]` and `[data-value="a b"]` stay distinct -- which
is the reason the context copy keeps its strings at all.
- Channels and alpha use the CSS <number> grammar. `[\d.]+` also matched
`.` and `1..2`, which parseFloat turned into NaN and a truncated 1 and
which were handed back as resolved -- the failure the hex grammar
check already prevents. It also rejected the legal `+255` and `2.55e2`.
- The rgb()/rgba() grammar crosses newlines. Upstream uses the `s` flag;
REX targets es2017, where that is a compile error, so this uses
[\s\S].
18 spec cases, 13 of which fail against the previous parser. None of
these constructs appear in REX's stylesheets, so theme.baseline.json on
#3133 is unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* CORE-2731: use the dotall flag, matching ui-components
I claimed in review that REX's es2017 target made the `s` flag a
compile error and used [\s\S] instead. That was wrong: flag validation
landed in TypeScript 5.5 and REX is on 4.9.5, and the flag is a runtime
feature that the target does not gate anyway.
So this matches ui-components#149 verbatim, which is the point -- the
copy is deleted by CORE-2737 and every needless difference is friction
for that swap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…theme-tokens # Conflicts: # src/test/cssColors.spec.ts # src/test/cssColors.ts
TomWoodward
requested a deployment
to
rex-web-core-2731-globa-fydzwf
September 11, 2026 16:29
Pending
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jira: CORE-2731 (sub-task of CORE-1685)
Follows ui-components#143 (CORE-2720), which did the same thing there.
2 of 2 — split at Roy's request, the same way #143 was split in ui-components. #3142 carried part 1 and has merged, so this now targets
maindirectly.main, and this branch takes it unmodified.The split was along a file boundary that already existed —
fsandpathwere used only by the theme-aware half, and that half's tests never needed the parser's. Moving it out regeneratedtheme.baseline.jsonbyte-identical, which was the check that it changed no behaviour. The parser's own review fixes landed onmainwith #3142 (theurl()name boundary and its quote-aware scan, whitespace collapsed only outside selector strings, the shared CSS<number>grammar,device-cmyk); merging main in brings them here and the baseline is unmoved by them.Why
The styled-components migration has been copying theme values into CSS by hand. Measured on
mainat f00f987:theme.ts—#42424234× across 26 files,#fff26× across 16,#d5d5d518×. (The audit finds 184, because it also catches barewhitekeywords and colors inside shorthands that a hex grep misses; 206 after the stylesheets main has landed since.):rootblock intosrc/index.css, referenced 63 times against those 154 literals. Colors only — no z-index or padding tokens — and nothing kept it in step withtheme.ts.Links.constants.tsdeclareslinkColor = '#027EB5'andlinkHover = '#0064A0'— the two most common "off-theme" colors in the CSS at 17 occurrences each, every CSS copy lowercase against the uppercase TS original.content/components/constants.tswas a third (mainContentBackground = '#fff').What this does
Splits the theme data out.
theme.tsimportscssfromstyled-components/macrofor thebreakpoints.mobile()helpers, so a build-time generator importing it would drag styled-components into the build. The pure data —color,padding,zIndex, the breakpoint sizes — moves tosrc/app/themeData.tswith no imports at all.theme.tsspreads it back into its default export, so everytheme.color.xpath and all ~200 consumers are untouched. This is a step the epic needs regardless;theme.tsis one of the 26 files still on styled-components.Generates the token file.
src/app/themeCss.tsowns the projection;src/app/theme.cssis generated from it, never hand-edited. 80 tokens:--color-*(kebab-case of the JS path, so all 21 existing names are preserved exactly),--z-index-*(the 15 computed keys),--padding-page-*. Hex is lowercased on the way out, so a token's value has one spelling andcolor-hex-caseis satisfied without touching the published JS values.script/generate-theme-css.tsruns vianode ./script/entry generate-theme-css— the repo's existing TS-script idiom, so unlike ui-components no tsc-into-a-temp-dir dance was needed. Wired intobuildandprestart. Deliberately not intobuild:css:pretestrunsbuild:css, so generating there would regenerate the file immediately before the suite and make the freshness check pass vacuously.Absorbs the other sources. Link colors live in
themeData.tsand are re-exported byLinks.constants.ts, so its 8 JS importers are unchanged but there is now one source and three tokens.mainContentBackgroundpoints at the theme's white.Enforces it.
src/app/theme.spec.ts:theme.cssequals the generator's output — one equality, so a missing token, an orphan token and a stale value all fail the same way;--color-*/--z-index-*/--padding-*token that does not exist;(5) found two real cases:
BookBanner.cssread--z-index-bigand--z-index-mini, component-local variables sitting in the global family's namespace. They are derived from--z-index-navbar, so CSS now computes them withcalc()and the two JS bindings are gone.(6) is the cheap mitigation for the breakpoint gap.
@media (min-width: var(--x))is not valid CSS, so75emstays duplicated 144 times. Banning component-specific breakpoints would be wrong — Footer legitimately uses 37.5em, 60.1em and 90em — so instead it catches the failure the duplication actually causes: a value meant to be a theme breakpoint but mistyped as74em, which silently stops matching.The baseline is a ratchet, not a skip. Checks (3) and (4) are locked to
src/app/theme.baseline.json— 206 duplicated literals and 37 unrecognised colors as of this commit. So enforcement starts now: a new hardcoded#424242fails CI today, and the list can only shrink as the sweep PRs land.yarn generate:theme-baselineregenerates it and prints the counts, which should only go down. This is why the sweep is not in this PR: it is ~50 files of values-only edits and wants reviewing against screenshots separately.The theme-aware half of the audit
src/test/themeColors.tsis what sits between #3142's parser and the enforcement above, and it is where the interesting decisions are:themeColorIndexnames every token that carries a color, not one. Ten of the theme's colors are carried by more than one token —#fffby eleven,#424242by six,#000by five — so picking one is a guess presented as an instruction, and it was already guessing wrong (see the review rounds below).occurrenceis what the baseline locks. A color occurrence is identified by the declaration it was written in — file, selector and at-rule, property — rather than by file and literal, so removing one#fffand adding another elsewhere in the same file cannot leave the baseline unchanged. Deliberately not a line number: that churns the baseline whenever an unrelated rule is inserted above one, and a baseline regenerated for an unrelated reason is exactly where a new color hides.KNOWN_OFF_PALETTEis empty and each future entry needs a reason. A color only belongs there if snapping it to the nearest palette entry would be a visual change — a design decision, not a refactor.script/generate-theme-baseline.ts, so the baseline cannot be generated by different logic than it is checked with.Verification
palette.blackwithout regenerating fails the spec; regenerating passes it.theme.spec.tshas a guard that the file walk found something, so the suite cannot pass vacuously.rgba(0, 0, 0, 0.2)is black at 20% and has no token form) while still refusing a new hue throughrgba(), and fails rather than passing silently on anything it cannot resolve to channels.calc()z-indexes, which resolve to the same 69 and 68 as before, verified in Chrome.Status
Ready for review, behind #3142. The sweep follows in separate PRs under the same ticket.
This PR races main. Every plain-CSS migration that merges before it lands adds literals the check is not yet on main to stop, which fails the ratchet until the baseline is regenerated — it has happened once already. The remedy is one command, but it is an argument for landing this sooner rather than later.
Not yet done: the screenshot suite. No visual change is intended and the only rendered-output change is BookBanner's two
calc()z-indexes, which I have verified in Chrome resolve to the same 69 and 68 as the bindings they replaced — but that is reasoning plus a unit check, not a pixel diff, so the suite is still worth running before merge.Review rounds
Copilot found nine issues across four rounds and Roy found a failing test; all are fixed. The five that were in the parser are now in #3142 and are described there — the percentage-channel mis-keying,
#gggparsing as a color, named colors read outside color positions, and the two holes in how the baseline identifies an occurrence. The ones that belong to this half:{added, removed}rather than asserting on two sorted 200-entry arrays, so a failure names the occurrences that moved. The directions mean different things:addedwants a token,removedwants the baseline tightened around it.#ffftwice in one declaration, so the baseline holds that key twice, and set membership would call 2 → 1 no change and let a fixed violation come back for free. Compared as multisets instead, with the comparison itself under test.themeColorIndexkept one token per color value, so the last in the projection was reported as the replacement — and it was already misattributing: every#000in the baseline was blamed on--color-text-black, including.nudge-click-blocker { background-color }(a full-screen overlay) and.button-primary:focus { box-shadow }, both of which mean--color-black. It now names all of them, least-qualified first, and the author picks. This is the sibling of the--ox-color-linkfix in ui-components#143.… is --color-white), which made renaming a token or giving an existing value a new token rewrite every affected baseline line — churn for a reason unconnected to the CSS, and by this PR's own argument that is where a new color hides. Entries are now identity only; the tokens are recomputed and attached toaddedentries when a check fails.🤖 Generated with Claude Code